Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.4] Add support for missing Curl methods to the Curl client #39471

Open
wants to merge 8 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

lbajsarowicz
Copy link
Contributor

Description (*)

Cherry-picked change from #39330 rated as "backwards-incompatible" by @engcom-Dash rejecting all the changes to Testing Framework, leaving just the new methods in the Curl class.

It makes me upset that after spending hours to get the previous PR in shape to merge, this bug fix was taken down instead of recommendation to fix imaginary "backwards incompatiblity", provided Maintainers can point what change became backwards-incompatible.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Copy link

m2-assistant bot commented Dec 12, 2024

Hi @lbajsarowicz. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@lbajsarowicz
Copy link
Contributor Author

@magento run all tests

@lbajsarowicz
Copy link
Contributor Author

@magento run all tests (after changing the method to pass payloads)

Copy link

Failed to run the builds. Please try to re-run them later.

@bgorski
Copy link
Contributor

bgorski commented Dec 12, 2024

@magento run all tests

@lbajsarowicz
Copy link
Contributor Author

@magento run all tests

I simplified the delete method signature to make the method compatible to the Magento\TestFramework, which - according to @engcom-Dash (in #39330) has higher priority than Magento\Framework.

@engcom-Charlie
Copy link
Contributor

Hi @lbajsarowicz,

Did you get chance to look into these review comments.

@lbajsarowicz
Copy link
Contributor Author

Update Looking into it.

Copy link
Contributor

@Den4ik Den4ik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lbajsarowicz,
Thanks for your work.
In my opinion this is really good candidate for 2.4.8 release.

@engcom-Charlie
Copy link
Contributor

engcom-Charlie commented Mar 14, 2025

Hi @lbajsarowicz,

As mentioned here can you please work on the review comments. I have worked on it but unable to push the changes because of access rights issue to your repository.

Please do the needful so that we can able to proceed further on this PR. Till then moving this PR to On Hold.

@lbajsarowicz
Copy link
Contributor Author

@engcom-Charlie I'm working on it now.

@lbajsarowicz
Copy link
Contributor Author

@magento run all tests

Copy link
Contributor

@bgorski bgorski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbajsarowicz thank you for the contribution!
@engcom-Charlie with the $params issue handled, this is ready to get processed further.

@bgorski bgorski requested a review from engcom-Hotel March 14, 2025 13:24
@lbajsarowicz
Copy link
Contributor Author

@engcom-Charlie I've discussed the $params with @bgorski offline and we have agreed that it's easier to change the implementation to support $params in the future, if there's such a case, rather than modifying already published API for delete method (ergo: Look how many issues the Integration Tests framework caused).

@engcom-Charlie
Copy link
Contributor

@engcom-Charlie I've discussed the $params with @bgorski offline and we have agreed that it's easier to change the implementation to support $params in the future, if there's such a case, rather than modifying already published API for delete method (ergo: Look how many issues the Integration Tests framework caused).

Okay @lbajsarowicz. Thank you! I was about to ask for review from @bgorski after the changes but thank you @bgorski for doing it prior 👍

@engcom-Charlie
Copy link
Contributor

As we have got the review approval from @bgorski moving this PR to Ready for testing.

@engcom-Charlie engcom-Charlie added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Mar 14, 2025
@engcom-Charlie
Copy link
Contributor

@magento run all tests

@Den4ik
Copy link
Contributor

Den4ik commented Mar 15, 2025

Returning to the question of using $params, this parameter does not serve any specific purpose and is misleading.

I have an image located at domain.test/media/catalog/product/some_product.jpg. I use Lambda and S3 for image resizing, for example:
domain.test/media/catalog/product/some_product.jpg?width=255&height=255.

Let's say I want to delete an image of a specific size but keep the original. I see that the delete method accepts parameters, so I call:

delete('domain.test/media/catalog/product/some_product.jpg', ['width' => 255, 'height' => 255]);

As a result, I end up with something completely different from what I expected.

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Mar 15, 2025

I understand your idea - but still the standard mentions the existence of params. Imagine that one day Google introduces this feature of the standard in their APIs.

Change to Magento would be more complicated than introducing that for 8 billion Internet users around the world, as this would require method signature changes.

This is also why we (with @bgorski) agreed after discussion that the 2nd argument should be default to [] to avoid unnecessary confusion.

@Den4ik
Copy link
Contributor

Den4ik commented Mar 15, 2025

@lbajsarowicz In this case some documentation that $params are not used in request can help avoid misleading developers. What do you think?
cc @bgorski

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@engcom-Charlie
Copy link
Contributor

Manual testing is not required here, as this PR is taking care of few missed CURL methods to existing CURL client. Since the builds are failing, moving it to Extended Testing.

@engcom-Charlie
Copy link
Contributor

@magento run Database Compare, Functional Tests B2B, Functional Tests CE, Functional Tests EE, Semantic Version Checker, Static Tests, WebAPI Tests

@engcom-Charlie
Copy link
Contributor

@lbajsarowicz can you please have a look into Static test failure.

For SVC failure, I have raised internal approval JIRA. Moving this PR to Pending Approval now. We will proceed ahead on this once will get all the required approvals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P3 May be fixed according to the position in the backlog. Progress: pending approval Project: Community Picked PRs upvoted by the community Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Status: Pending Approval
Development

Successfully merging this pull request may close these issues.

7 participants